Support ALTER MATERIALIZED VIEW ... SET PROPERTIES ... by sunningCGo · Pull Request #9613 · trinodb/trino

sunningCGo · 2021-10-13T04:30:03Z

This PR implements support for the ALTER MATERIALIZED VIEW ... SET PROPERTIES ... statement

lukasz-stec

Would be good to have high-level test like BaseConnectorTest.testRenameMaterializedView

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java

sopel39 · 2021-10-13T12:29:13Z

Please base your PR on top of #9401

sunningCGo · 2021-10-14T04:34:02Z

#9401 has been merged into master and I have rebased this PR onto master.

sopel39

lgtm % comments

core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

core/trino-main/src/main/java/io/trino/security/AccessControl.java

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java

sopel39

lgtm % comments

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java

electrum

There are some problems with the existing ALTER TABLE ... SET PROPERTIES that need to be resolved, so we need to hold off on this until that is done.

sopel39 · 2021-10-15T21:29:19Z

There are some problems with the existing ALTER TABLE ... SET PROPERTIES that need to be resolved, so we need to hold off on this until that is done.

@electrum what problems do you mean?

core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java

sopel39

lgtm % comments. Previously you had tests for add XXX with (prop=DEFAULT) where you checked that it fails. I think these tests were valuable (now they should pass)

core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java

testing/trino-tests/src/test/java/io/trino/tests/TestMockConnector.java

core/trino-main/src/test/java/io/trino/execution/TestCreateMaterializedViewTask.java

testing/trino-tests/src/test/java/io/trino/tests/TestMockConnector.java

Old

sopel39

lgtm % comments. @martint do you want to look?

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java

core/trino-main/src/main/java/io/trino/metadata/AbstractCatalogPropertyManager.java

core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java

core/trino-parser/src/main/java/io/trino/sql/tree/Property.java

This commit implements ALTER MATERIALIZED VIEW ... SET PROPERTIES... in the core engine. The implementation supports the use of a newly introduced SQL keyword "DEFAULT" on the right hand side of a property assignment. The support of the DEFAULT keyword is implemented by modifying certain classes related to properties in general (i.e. they are not specific to materialized view). Consequently, several other SQL statements also gain the support of the DEFAULT keyword "for free". They are: ALTER TABLE... ADD COLUMN... ANALYZE CREATE MATERIALIZED VIEW CREATE SCHEMA CREATE TABLE (both column properties and table properties) CREATE TABLE AS Note, however, that ALTER TABLE ... SET PROPERTIES ... is not in the list above. Support for DEFAULT in ALTER TABLE...SET PROPERTIES... requires extensive changes and will therefore be implemented in a subsequent commit.

There are two versions of AccessControl.checkCanCreateTable: the old deprecated version and the newer version which takes an additional Map<String, Object> parameter "properties"(so that the properties can be taken into account when making an access control decision). Which version is to be used should depend solely on the FeatureConfig parameter "disableSetPropertiesSecurityCheckForCreateDdl" - it should not depend on whether the "properties" parameter is empty.

cla-bot bot added the cla-signed label Oct 13, 2021

sunningCGo requested a review from sopel39 October 13, 2021 04:30

sunningCGo changed the title ~~Implement ALTER MATERIALIZED VIEW ... SET PROPERTIES ...~~ Support ALTER MATERIALIZED VIEW ... SET PROPERTIES ... Oct 13, 2021

sunningCGo force-pushed the alter-mv-set-props branch from 3557a83 to cf37d85 Compare October 13, 2021 05:15

sopel39 requested a review from lukasz-stec October 13, 2021 08:26

lukasz-stec reviewed Oct 13, 2021

View reviewed changes

electrum added the syntax-needs-review label Oct 14, 2021

electrum requested a review from martint October 14, 2021 01:52

sunningCGo force-pushed the alter-mv-set-props branch from bd6a521 to 6c268c7 Compare October 14, 2021 04:17

sunningCGo force-pushed the alter-mv-set-props branch from c837fbc to f495d80 Compare October 14, 2021 05:47

sopel39 reviewed Oct 14, 2021

View reviewed changes

sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 30d5126 to b97ad90 Compare October 15, 2021 01:09

sunningCGo requested a review from sopel39 October 15, 2021 01:16

sopel39 reviewed Oct 15, 2021

View reviewed changes

sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 36331ed to 61b8681 Compare October 15, 2021 16:09

sunningCGo requested a review from sopel39 October 15, 2021 16:20

electrum previously requested changes Oct 15, 2021

View reviewed changes

sunningCGo force-pushed the alter-mv-set-props branch from 61b8681 to bdfdfec Compare October 15, 2021 20:58

sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 77ff9f3 to f6b9ad6 Compare October 27, 2021 00:56

sunningCGo force-pushed the alter-mv-set-props branch 3 times, most recently from 1d1a6ba to cd3beca Compare November 2, 2021 14:10

sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 05d60cf to 6afac02 Compare November 22, 2021 05:19

sopel39 reviewed Dec 14, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java Outdated Show resolved Hide resolved

sunningCGo force-pushed the alter-mv-set-props branch 6 times, most recently from 205d3e1 to 05c1100 Compare December 15, 2021 04:43

sopel39 approved these changes Dec 15, 2021

View reviewed changes

sunningCGo force-pushed the alter-mv-set-props branch from 05c1100 to ae56504 Compare December 16, 2021 00:25

sunningCGo requested a review from sopel39 December 16, 2021 01:27

sunningCGo closed this Dec 20, 2021

sunningCGo deleted the alter-mv-set-props branch December 20, 2021 02:10

sunningCGo restored the alter-mv-set-props branch December 20, 2021 02:12

sunningCGo reopened this Dec 20, 2021

electrum requested review from electrum and removed request for electrum January 15, 2022 01:07

sunningCGo force-pushed the alter-mv-set-props branch from ae56504 to 848b17a Compare January 18, 2022 02:27

github-actions bot added the tests:hive label Jan 18, 2022

sunningCGo force-pushed the alter-mv-set-props branch from 848b17a to c81a4ea Compare January 18, 2022 04:13

sopel39 approved these changes Jan 18, 2022

View reviewed changes

sunningCGo force-pushed the alter-mv-set-props branch from c81a4ea to 5b4c667 Compare January 18, 2022 20:25

sopel39 reviewed Jan 19, 2022

View reviewed changes

core/trino-parser/src/main/java/io/trino/sql/tree/Property.java Outdated Show resolved Hide resolved

sunningCGo mentioned this pull request Jan 19, 2022

Model DEFAULT as a type of expression #10687

Open

Sunning Go added 3 commits January 24, 2022 16:14

Add extension point for setting mv properties in Hive connector

842d8b2

sopel39 mentioned this pull request Jan 25, 2022

Release notes for 369 #10552

Closed

This was referenced Jan 25, 2022

Add Trino 369 release notes #10553

Merged

Document ALTER MATERIALIZED VIEW/TABLE ... SET PROPERTIES #10774

Closed

Conversation

sunningCGo commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukasz-stec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 commented Oct 13, 2021

Uh oh!

sunningCGo commented Oct 14, 2021

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

sopel39 commented Oct 15, 2021

Uh oh!

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

sunningCGo commented Oct 13, 2021 •

edited

Loading